-
Notifications
You must be signed in to change notification settings - Fork 123
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for #derive[FromStr] for enums #176
Conversation
type Err = #err_name; | ||
#[inline] | ||
fn from_str(src: &str) -> ::core::result::Result<Self, Self::Err> { | ||
Ok(match src.to_lowercase().as_str() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I considered adding .replace(&[' ', '_', '-'], "")
here so that it would mach against kebab-case and SNAKE_CASE too, but I'm not sure how lenient it should be, I am already on the fence about the case insensitive matching.
quote! { | ||
|
||
#[derive(Debug, Clone, PartialEq, Eq)] | ||
#visibility struct #err_name; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I like creating a type out of nothing here, but I did not like the other options better:
- could use
&'static str
but that does not impelementstd::error::Error
which makes it annoying for combining with other errors - could have a
derive_more::ParseError
but that complicates the dependencies as I don't think you can have code and procmarcos in the same dependency
4c17d26
to
ec7c52b
Compare
Forced pushed changes that hopefully fix the failing tests |
ec7c52b
to
479a0c4
Compare
@@ -51,7 +51,7 @@ deref_mut = [] | |||
display = ["syn/extra-traits"] | |||
error = ["syn/extra-traits"] | |||
from = ["syn/extra-traits"] | |||
from_str = [] | |||
from_str = ["convert_case"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should fix the current compilation issue in the feature tests.
from_str = ["convert_case"] | |
from_str = ["convert_case", "syn/extra-traits"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I just forced pushed a change that bumps syn
to 1.0.81 (which is the version in the lock file) and changed the code to use .is_empty() instead which means it does not need the extra traits
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bah looks like I also need to ditch the x == from_str(x.to_string())
test as I can't #derive[Display]
if the display feature is not set.
8607fa0
to
c4190a8
Compare
Is there anything blocking this PR from progressing? |
The test are all now passing and I'm happy with my commits as they stand, @JelteF let me know if there are any more changes that you want me to make. |
for variant in variants { | ||
let variant_str = variant.to_string(); | ||
cases.push(quote! { | ||
#canonical if(src == #variant_str) => #input_type::#variant, | ||
}) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this edge case is missing a test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is good except for a missing test for the smart case sensitive behaviour.
This allows deriving FromStr for enums with no fields in the obvious way fixes JelteF#59
c4190a8
to
d15513e
Compare
I have added |
Thanks a lot for your work on this! |
This allows deriving FromStr for enums with no fields in the obvious way
fixes #59